Skip to content

[MSCRIPTING-7] binding the session and servers helper in binding context - #4

Open
rmannibucau wants to merge 1 commit into
masterfrom
MSCRIPTING-7
Open

[MSCRIPTING-7] binding the session and servers helper in binding context#4
rmannibucau wants to merge 1 commit into
masterfrom
MSCRIPTING-7

Conversation

@rmannibucau

@rmannibucau rmannibucau commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MSCRIPTING-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MSCRIPTING-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@rmannibucau rmannibucau changed the title binding the session and servers helper in binding context [MSCRIPTING-7] binding the session and servers helper in binding context Aug 6, 2021
bindings.put( "session", session );
bindings.put( "project", project );
bindings.put( "log", getLog() );
bindings.put( "servers", new Servers( session, settingsDecrypter ) );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have doubts about this one, servers has never been used as a variable. Better to stick to settings with resolved values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have it already? I would like to avoid to decipher the whole settings each time (this is done only when needed) because it can slow down the execution a loooot for nothing in practise.
Happy to wrap it in a MavenScripting wrapper which would expose this kind of API (so script would use mavenScripting.servers.find(...))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand what you need and staying close to what Maven offers, I think you should focus on session.getRepositorySession().getAuthenticationSelector().getAuthentication(RemoteRepository)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I want is a Server#password in clear but I don't want to decipher all servers upfront. From what I saw, getSettings() is NOT clear there so I would have to "clearify" it before binding it which I want to avoid so I thought using a lazy simple accessor is more script friendly than exposing the session and letting user use plexus container to do lookups to decipher values.

@rfscholte

Copy link
Copy Markdown
Contributor

Don't forget to update https://github.com/apache/maven-scripting-plugin/blob/master/src/site/markdown/script-context.md

@rfscholte

Copy link
Copy Markdown
Contributor

I'm going to close this one as I consider this a security vulnerabilty. The introduction of servers with exposed username/password means you can easily extract the credentials from the system it is running on.

@rfscholte rfscholte closed this Aug 21, 2021
@asfgit
asfgit deleted the MSCRIPTING-7 branch August 21, 2021 17:29
@rmannibucau

Copy link
Copy Markdown
Contributor Author

@rfscholte will you also prevent using any mojo? It is the same security level. Without that this plugin is not that helping in most cases IMHO (no easy website custo/generation), no CI/CD etc...
Worked around this lack by using exec plugin and a custom maven deceypter - it is just 100LoC but the security point is clearly not accurate for any mojo since they all have access to it and they are all - script or not - part of the build. Preventing a script not in the project is a thing but not all other cases technically speaking.

@bmarwell

bmarwell commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

You could also add:

         bindings.put( "pluginDescriptor", pluginDescriptor );
         bindings.put( "mojoExecution", mojoExecution );

from #7 if you think it adds any benefits.
Needs a rebase and spotless:apply either way.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

@bmarwell this is true but this is also something the script somehow know in the context of a project. What would be more useful is a replacement for plexus container to lookup any potentially needed bean or a loose coupling like in exec:java (constructor injection runnable case) but my suspicion is maven (4) misses a container handler for these cases.

@rmannibucau
rmannibucau restored the MSCRIPTING-7 branch March 4, 2024 20:00
@bmarwell

bmarwell commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Re-opened for new evaluation

@bmarwell bmarwell reopened this Mar 4, 2024
@cstamas

cstamas commented Mar 4, 2024

Copy link
Copy Markdown
Member

A big -1 here

@rmannibucau

Copy link
Copy Markdown
Contributor Author

@cstamas any rational behind and proposal to solve the related issue (enable to do proper resolutions from the script)?

@cstamas

cstamas commented Mar 4, 2024

Copy link
Copy Markdown
Member

This is a matter of principle, and IMHO, vision.

Plugins like these are like "swiss knives" and allows hacking, in effect, like Ant or other tools allow it (and makes Maven hackable just like one would do with Ant).

Maven IMHO should be more like "set of Un*x tools" instead: set of battle tested (but also simple) plugins, that do few things, but do them well.

If someone is akin for "swiss knife" plugins like these, for me is clear sign that they want to even widen their "hacking", that is IMHO another sign, they should be using Gradle, Ant or whatever else. Maven is a declarative build system.

And I did not even mention the reasons Robert brought up.... (plus multitude of other reasons).

@bmarwell

bmarwell commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

This is a matter of principle, and IMHO, vision.

Plugins like these are like "swiss knives" and allows hacking, in effect, like Ant or other tools allow it (and makes Maven hackable just like one would do with Ant).

Maven IMHO should be more like "set of Un*x tools" instead: set of battle tested (but also simple) plugins, that do few things, but do them well.

If someone is akin for "swiss knife" plugins like these, for me is clear sign that they want to even widen their "hacking", that is IMHO another sign, they should be using Gradle, Ant or whatever else. Maven is a declarative build system.

And I did not even mention the reasons Robert brought up.... (plus multitude of other reasons).

This is also already possible in the exec plugin, and no one said this was a vuln (see Romains comment). Then, this would mean you need also get rid of exec-plugin.

Then, what about the requirements history?

Why was this plugin released in the first place? There could have been a vote against it at that time?

@cstamas

cstamas commented Mar 4, 2024

Copy link
Copy Markdown
Member

It is not all black or white of course.

We do need some "backdoors" open, for case of "emergency", if no plugin exists that does what you need. But if you google it, you will see the trend: "use hack in maven, but if you need it too often, write a plugin for it" tendency. And we must not lose this.

This is why originally writing maven plugins was possible using ant scripts and beanshell as well. Beanshell, Ant script, they were all there to lower the plugin authoring barrier, BUT, those were plugins, they used plugin API, had descriptors, etc, so any future thing like build avoidance, incremental build (real one, not what we have today), etc. could rely on them.

Problem is when "hacks" remain hacks, or worse, when "hacks become pattern" (of using some "special" (backdoor) plugin). So if you have ant/exec plugin in build, it is okay, as long as you do not stick with it. But in our case, I see pattern of building some "library of hacks" on top of these, that makes ANY of these "solutions" non reusable, non shared, not maven plugins

If you need something special, hack it once. But second time write a plugin for it. Period.

In short, we do need "stepping stone" plugins (hacks, like exec, ant plugins) to solve some ad-hoc scenarios, BUT these are stepping stones, in a sense, if pattern is repeated, they should be made into reusable plugins ultimately, while I see no intention of that happening here. In contrary, I see quite the opposite intent.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

@cstamas I'm on the camp to enable people to do what works for them, for me maven key is dependencies and it must stay static, rest (build pipeline) already broke maven original design and convention and it is bad to need to create a module/project for a build need so I'm clearly keen to see this kind of thing happening. No strong push from me there since I moved to exec and some companions but think it is sane.

@bmarwell

bmarwell commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I'm somewhere between the lines here. If you have a common use case, a dedicated plugin is probably a better solution compared to the exec-, antrun-, or scripting-plugin.

I'll say let's skip this pr for now... There's also #7 FWIW.
If more people need such a backdoor, we can visit this and #7 again.

private MavenProject project;

// script variables
@Parameter(defaultValue = "${session}", readonly = true)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@component can replace read-only parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a pro? I tend to prefer to limit the API (#imports) but if there is some advantage i'll do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply change to @Inject

@slachiewicz
slachiewicz removed their request for review July 28, 2024 11:01
@jira-importer

Copy link
Copy Markdown

Resolve #44

@elharo

elharo commented Dec 21, 2025

Copy link
Copy Markdown
Contributor

There are some pretty heavy -1's on this, and no approvals so I'm going to close this one as rejected.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

Tempted to say it should be reopened or another fix be proposed cause as of today the scripting plugin stays hard to use without that making exec superseeding scripting plugin.

For security point: it is mainly convenience since everything is available while the eval impl doesn't fork.

FTR exec plugin supports it out of the box but requires to compile something before which isnot as neat as scripting plugin (a module/project or the main module if the build classes are excluded in jar plugin): https://github.com/mojohaus/exec-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/exec/AbstractExecJavaBase.java#L518.

@slachiewicz slachiewicz reopened this Dec 22, 2025
@slachiewicz slachiewicz reopened this Dec 29, 2025
@slachiewicz

Copy link
Copy Markdown
Member

Lets keep it open for future. Maybe we will have more voiced or something will change in mean time.
I have no strong opinion which way to go - similar with Gradle it's possible to write any code and access credentials in Groovy

@rmannibucau

Copy link
Copy Markdown
Contributor Author

Hi guys, can you review it again?
We just got a release and this 4 yo PR missed it.
All the -1 I see are "it is bad" but we do these bad things in pretty much all plugins so tempted to say it doesn't hurt more and ultimately we delegate the "bad" impact on the user land only.
I'm pushing it a bit because it enables so much cases and solve all the groovy/external-scripting issues by using java which is a big pro.
In current form we just need to hack maven to grab the session by reflection which is worse IMHO.

@elharo

elharo commented Dec 29, 2025

Copy link
Copy Markdown
Contributor

Tests are failing. Fixing that would be required for review. I don't have a particular opinion on this, but I note that other people whose opinion I respect are all negative about this, so I don't hold out high hopes here. It's uncommon for a PR to get multiple informed -1 votes so it's probably worth paying attention to.

@rmannibucau
rmannibucau force-pushed the MSCRIPTING-7 branch 2 times, most recently from 430872f to abd7db1 Compare December 29, 2025 14:56
@rmannibucau

Copy link
Copy Markdown
Contributor Author

Ok so build should be green now :)

@cstamas while I agree on your phrasing I also met too often these cases (a lot around living doc and living deployments, rarely on the build phase itself - the side note there is it is often specific to the infra/project) to be happy with "write a plugin" in current form today and we'll not solve plugin entry cost until at least maven 6 I think so can you revise your -1 to at least a +0?

import org.apache.maven.settings.crypto.SettingsDecryptionResult;
import org.codehaus.plexus.PlexusContainerException;
import org.junit.jupiter.api.Test;
import org.sonatype.plexus.components.cipher.DefaultPlexusCipher;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we using org.sonatype instead of org.codehaus.plexus?

@DavoBR

DavoBR commented May 15, 2026

Copy link
Copy Markdown

Hi there! I’ve been following this PR and I think exposing the MavenSession instance is a crucial addition for this plugin. It would significantly increase the scripts' capabilities by allowing access to project properties, settings, and the reactor state in a straightforward way.

I personally prefer this plugin over others like gmavenplus due to its simplicity and 'Maven-native' feel. Is there anything

I can help with to move this forward? It would be great to see this merged and released. Thanks for the effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants